Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolver transform to emit imports for helper and modifiers that need… #1806

Merged
merged 8 commits into from
Apr 2, 2024

Conversation

void-mAlex
Copy link
Collaborator

… them in strict mode templates

mansona
mansona previously approved these changes Feb 17, 2024
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me 👍 I think that this is in line with what we discussed in meetings and the fact that tests are passing gives me the confidence that this can be merged and released without much issue 👍

@void-mAlex void-mAlex requested a review from ef4 February 19, 2024 13:45
@ef4
Copy link
Contributor

ef4 commented Feb 20, 2024

I pushed the above commits to clarify the code and explore how much was already correct here. By unifying the builtInKeywords list the types uncovered a case that wasn't handled (the use of built-in in ambiguous forms).

But more changes are still needed here. The built-ins don't always win. For example, if you make app/components/input.hbs in a classic app, you'll see that it defeats the ember-provided <Input />.

Helpers are a little better. If you try to create your own app/helpers/fn.js it will throw when you try to invoke it. But there is also the problem of version skew. uniq-id was introduced in ember 4.4, but embroider stable supports back to 3.28. On 3.28, uniq-id might actually come from a polyfill addon.

Whatever decisions resolver-transform makes get cached in the babel-cache, and will endlessly troll people if the answers can change based on other side-effects in the system. Like upgrading your ember version across the 4.4. boundary. That is a big reason why we split the component/helper/modifier resolution out of here and into module-resolver.

All of the above are reasons why I don't think we can hard-code these answers in resolver-transform. Rather, resolver-transform should delegate these cases to module-resolver, where we have much more information about what is available across the app and all addons, and where the answers won't live inside the babel cache.

For example, I think we would entirely remove input from the builtInKeywords list here and instead update module-resolver so it resolves #embroider_compat/components/input and #embroider/compat/ambiguous/input to the virtual content export { Input as default } from '@ember/component'.

I realize that might seem like it adds extra work for your codemod use case, but I don't think it does. The codemod is already going to need to know how to follow a reexport like that, because effectively 100% of addon-provided components will go through similar reexports, and the codemod will need to follow through them. That is, <PowerSelect /> becomes an import from #embroider_compat/components/power-select which resolves to your-app-name/components/power-select which resolves to a file containing export { default } from 'ember-power-select/components/power-select'. It's this last specifier that the codemod wants to output.

@void-mAlex
Copy link
Collaborator Author

Thank you for this @ef4 ,
I do have to point out that in my work I've found that #embroider_compat/components/power-select resolves directly to a file in the re-written packages and not something from the app scope, so I'm not convinced about the addon provided components needing this treatment as I have tested this scenario (though not very extensively) and it behaves differently - as far as I've seen I get straight to the path after the re-export but more testing is needed.

also uniqId is exported as uniqueId - I'll fix that up unless there is a specific reason the former one was used.

I'll work to fix up the tests as well

@mansona mansona dismissed their stale review February 27, 2024 16:23

more to do ❤️

@mansona mansona force-pushed the emit-imports-for-known-built-ins branch 2 times, most recently from a223c7a to 213f90f Compare April 2, 2024 15:53
@mansona mansona force-pushed the emit-imports-for-known-built-ins branch from 213f90f to 8bde7b9 Compare April 2, 2024 15:55
@mansona
Copy link
Member

mansona commented Apr 2, 2024

FYI I just rebased this against stable 👍

@ef4 ef4 merged commit 60441fa into stable Apr 2, 2024
201 checks passed
@ef4 ef4 deleted the emit-imports-for-known-built-ins branch April 2, 2024 17:43
@github-actions github-actions bot mentioned this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants